-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for multiple git resolver configurations #8263
Add support for multiple git resolver configurations #8263
Conversation
52b64a2
to
16592ca
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/test pull-tekton-pipeline-integration-tests |
/test pull-tekton-pipeline-go-coverage-df |
@piyush-garg: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-tekton-pipeline-alpha-integration-tests |
/test pull-tekton-pipeline-go-coverage-df |
@piyush-garg: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/test pull-tekton-pipeline-integration-tests |
/test pull-tekton-pipeline-go-coverage-df |
@piyush-garg: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL is not happy 😅.
So I am ok to get this merge but it doesn't feel like something that scale that much. It will work fine for small teams, but I do feel like bigger team that would require tens or hundreds of identifier would instead use the parameter. Maybe it's just a documentation issue.
/cc @afrittoli
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chitrangpatel, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
16592ca
to
dc348a1
Compare
The following is the coverage report on the affected files.
|
/hold |
Sorry to hold the PR, but since the structure of the configuration and name of the parameters are user facing, we should fix them before this is merge. @vdemeester @chitrangpatel WDYT |
No worries regarding this. Lets all approve and agree with user facing things, then we can proceed with merge |
dc348a1
to
6c85bff
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
6c85bff
to
d30340e
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
Hey @afrittoli @chitrangpatel @vdemeester I have done all the changes based on comments, please take a look now. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the updates.
I couple more comments, mostly cosmetic, some about error handling which I think needs to be addressed.
docs/git-resolver.md
Outdated
@@ -195,6 +191,114 @@ spec: | |||
value: Ranni | |||
``` | |||
|
|||
### Specifying Configuration for Multiple Git Providers | |||
|
|||
You can specify configurations for multiple providers and even multiple configurations for same provider to use in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I would recommend using impersonal sentences, i.e. instead of "You can specify...", "It is possible to specify configurations..." etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This will add support for providing multiple configurations in git resolver configmap Configurations can be provided using unique key and that key can be passed as a param in the taskrun/pipelinerun to resolver Old format is supported to provide backward compatibility and docs added in details about how to use the feature. Unit and e2e test added Fixes tektoncd#5487
d30340e
to
33d1398
Compare
@afrittoli I have addressed all the comments |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the updates!
Docs could use a couple of minor updates, but that can be done as a follow-up PR, good candidate for Hacktoberfest :)
/lgtm
|
||
### Example Configmap | ||
|
||
You can add multiple configuration to `git-resolver-configmap` like this. All keys mentioned above are supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: "you can..."
It is possible to specify configurations for multiple providers and even multiple configurations for same provider to use in | ||
different tekton resources. You need to first add details in configmap with your unique identifier key prefix. | ||
To use them in tekton resources, pass the unique key mentioned in configmap as an extra param to resolver with key | ||
`configKey` and value your unique key. If no `configKey` param is passed, `default` will be used. You can specify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: you can...
/hold cancel |
This is follow up of tektoncd#8263 to fix and improve docs of git resolver /kind docs
This is follow up of tektoncd#8263 to fix and improve docs of git resolver /kind documentation
This is follow up of #8263 to fix and improve docs of git resolver /kind documentation
This will add support for providing multiple configurations
in git resolver configmap
Configurations can be provided using unique key and that key
can be passed as a param in the taskrun/pipelinerun to resolver
Old format is supported to provide backward compatibility and
docs added in details about how to use the feature.
Unit and e2e test added
Fixes #5487
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes